Manage submissions speed #2303
Manage submissions speed #2303
Conversation
📝 WalkthroughWalkthroughSwitched the submissions index to DataTables server-side rendering. Controller now serves paginated/searchable/sortable JSON and a new row formatter. Client JS manages selection state, delegated events, dynamic row rendering, and batch-action enablement. View removed server-rendered rows and provides header + select-all UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as Browser (manage_submissions.js)
participant Server as Server (SubmissionsController)
participant DB as Database
rect rgba(230,245,255,0.8)
Note over User,Browser: Initial page load (HTML)
User->>Browser: GET /submissions
Browser->>Server: GET /submissions (HTML)
Server->>DB: Query scaffold data (limited)
DB-->>Server: Return scaffold
Server-->>Browser: Render index view (DataTables config + headers)
Browser->>Browser: Initialize DataTables (serverSide)
end
rect rgba(245,230,255,0.8)
Note over Browser,Server: DataTables requests rows
Browser->>Server: AJAX GET /submissions.json (draw,start,length,search,order)
Server->>DB: Query submissions with filters/sort/limit (may compute score)
DB-->>Server: Return rows
Server->>Server: format_submission_for_datatable per row
Server-->>Browser: JSON (data, draw, recordsTotal, recordsFiltered)
Browser->>Browser: createdRow + drawCallback -> render row HTML, checkboxes, actions
end
rect rgba(230,255,235,0.8)
Note over User,Browser: Selection & batch actions
User->>Browser: Click row checkbox / select-all
Browser->>Browser: toggleRow / toggleAllRows -> update selections, UI highlight
Browser->>Browser: updateSelectedCount() -> enable/disable batch actions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/assets/javascripts/manage_submissions.js (1)
558-562: Reset selections safely when paging.
selectedSubmissions.map(selectedSubmission => toggleRow(selectedSubmission, false));mutatesselectedSubmissionswhile you iterate it, so at least one previously selected ID survives. After changing pages, destructive operations (delete, excuse, etc.) still include that hidden submission. Iterate over a copy (or just clear the arrays) before toggling to keep selection state accurate.- selectedSubmissions.map(selectedSubmission => toggleRow(selectedSubmission, false)); + [...selectedSubmissions].forEach(selectedSubmission => toggleRow(selectedSubmission, false));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/codeql-analysis.ymlis excluded by!**/*.yml
📒 Files selected for processing (3)
app/assets/javascripts/manage_submissions.js(3 hunks)app/controllers/submissions_controller.rb(1 hunks)app/views/submissions/index.html.erb(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-09-20T20:46:52.775Z
Learnt from: evanyeyeye
Repo: autolab/Autolab PR: 2209
File: app/controllers/autograders_controller.rb:0-0
Timestamp: 2024-09-20T20:46:52.775Z
Learning: In the `AutogradersController`, ensure that the `download_file` method has `action_auth_level :download_file, :instructor` to restrict access to instructors.
Applied to files:
app/controllers/submissions_controller.rb
📚 Learning: 2024-06-10T19:38:42.782Z
Learnt from: jlge
Repo: autolab/Autolab PR: 2011
File: app/controllers/assessments_controller.rb:28-29
Timestamp: 2024-06-10T19:38:42.782Z
Learning: `destroy_no_redirect` in `app/controllers/assessments_controller.rb` is an internal method used within the controller and not a public endpoint. It should not be included in the `before_action :set_assessment` exclusion list.
Applied to files:
app/controllers/submissions_controller.rb
🧬 Code graph analysis (1)
app/controllers/submissions_controller.rb (4)
app/controllers/application_controller.rb (1)
action_auth_level(53-71)app/helpers/application_helper.rb (1)
computed_score(111-118)app/models/submission.rb (1)
final_score(327-335)app/models/assessment_user_datum.rb (1)
final_score(106-109)
KesterTan
requested review from
a team and
KesterTan
and removed request for
a team
dwang3851
force-pushed
the
manage-submissions-speed
branch
from
cfdbab4 to
a6f0c1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/assets/javascripts/manage_submissions.js (2)
527-531: Remove redundantselectedStudentCidsfiltering.Lines 528-530 already conditionally remove
selectedCidfromselectedStudentCidsonly when no other submissions from that student are selected. Line 531 then unconditionally removes it again with_.without(), which is redundant and could incorrectly remove CIDs that should remain selected.Apply this diff:
const hasOtherSelectedSubmissions = selectedSubmissions.some(id => submissions_to_cud[id] === selectedCid); if (!hasOtherSelectedSubmissions) { selectedStudentCids = selectedStudentCids.filter(cid => cid !== selectedCid); } - selectedStudentCids = _.without(selectedStudentCids, selectedCid); }
558-562: Reconsider clearing selections on pagination.This handler clears all selected submissions when the user navigates to a different page. Typically, users expect selections to persist across pages so they can select submissions from multiple pages for batch operations (e.g., regrade, delete, download).
Consider removing this handler to allow selections to persist, or add a clear indicator to users that changing pages will clear their selections.
♻️ Duplicate comments (1)
app/controllers/submissions_controller.rb (1)
42-42: SQL injection vulnerability remains unfixed.The
order_directionparameter is still taken directly from params without validation and later interpolated into SQL (line 84). This allows SQL injection attacks.As noted in the previous review, apply this fix:
- order_direction = params.dig(:order, '0', :dir) || 'desc' + direction_param = params.dig(:order, '0', :dir).to_s.downcase + order_direction = %w[asc desc].include?(direction_param) ? direction_param : 'desc'
🧹 Nitpick comments (1)
app/views/submissions/index.html.erb (1)
14-20: Remove unusedadditional_dataarray.With the migration to DataTables server-side processing, this
additional_dataarray appears unused. The JavaScript now buildssubmissions_to_cuddynamically from the JSON response (seemanage_submissions.jslines 243-248). The server-side loop over@submissionshere no longer serves a purpose.Apply this diff to remove the unused code:
- // additional data for each row - additional_data = [ - <% for s in @submissions do %> - { - "submission-id": "<%= s.id.to_s %>", - }, - <% end %> - ]; -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/assets/javascripts/manage_submissions.js(3 hunks)app/controllers/submissions_controller.rb(1 hunks)app/views/submissions/index.html.erb(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-06-10T19:38:42.782Z
Learnt from: najclark
Repo: autolab/Autolab PR: 2011
File: app/controllers/courses_controller.rb:1316-1320
Timestamp: 2024-06-10T19:38:42.782Z
Learning: The user confirmed the concern about potential security risks when setting `flash[:error]` and `flash[:html_safe]` with user-supplied input, specifically mentioning `course_name` derived from a tar file's contents. It's important to ensure proper sanitization of user-supplied input to prevent XSS vulnerabilities, especially when `flash[:html_safe]` is used, which indicates the content should not be escaped.
Applied to files:
app/controllers/submissions_controller.rb
📚 Learning: 2024-09-20T20:46:52.775Z
Learnt from: evanyeyeye
Repo: autolab/Autolab PR: 2209
File: app/controllers/autograders_controller.rb:0-0
Timestamp: 2024-09-20T20:46:52.775Z
Learning: In the `AutogradersController`, ensure that the `download_file` method has `action_auth_level :download_file, :instructor` to restrict access to instructors.
Applied to files:
app/controllers/submissions_controller.rb
📚 Learning: 2024-06-10T19:38:42.782Z
Learnt from: jlge
Repo: autolab/Autolab PR: 2011
File: app/controllers/assessments_controller.rb:28-29
Timestamp: 2024-06-10T19:38:42.782Z
Learning: `destroy_no_redirect` in `app/controllers/assessments_controller.rb` is an internal method used within the controller and not a public endpoint. It should not be included in the `before_action :set_assessment` exclusion list.
Applied to files:
app/controllers/submissions_controller.rb
🧬 Code graph analysis (1)
app/controllers/submissions_controller.rb (3)
app/models/submission.rb (1)
final_score(327-335)app/controllers/application_controller.rb (1)
action_auth_level(53-71)app/helpers/application_helper.rb (1)
computed_score(111-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (6)
app/views/submissions/index.html.erb (1)
120-147: LGTM: Header structure and DataTables placeholder.The streamlined header structure with the select-all checkbox, conditional sort icons, and DataTables-driven tbody placeholder is well-implemented and aligns with the server-side processing migration.
app/controllers/submissions_controller.rb (2)
19-34: LGTM: HTML format initialization.The HTML format branch correctly initializes instance variables and loads a limited set of submissions (100) for the initial page render. The
@submissions_to_cudmapping is efficiently created usingpluckandto_h.
105-137: LGTM: DataTables formatter method.The
format_submission_for_datatablemethod is properly authorized at the:instructorlevel and returns a well-structured array for DataTables consumption. The use of instance variables and helper methods is appropriate.app/assets/javascripts/manage_submissions.js (3)
76-96: LGTM: Delegated event handler.The use of
$(document).on('click', '.score-details', ...)is the correct pattern for handling clicks on dynamically created DataTables rows. This ensures the event handler works for elements rendered after page load.
236-306: LGTM: DataTables server-side configuration.The DataTables initialization with server-side processing is well-implemented:
- AJAX endpoint correctly targets the JSON format
dataSrccallback builds thesubmissions_to_cudmapping from response data- Column definitions properly handle rendering, sorting, and visibility
createdRowanddrawCallbackmaintain selection state across redraws
315-321: LGTM: Select-all checkbox synchronization.The
updateSelectAllCheckboxfunction correctly synchronizes the select-all checkbox state with individual row selections.
| } | ||
| } | ||
|
|
||
| $(document).ready(function () { |
There was a problem hiding this comment.
Can we remove these double document.ready functions
There was a problem hiding this comment.
i don't really know why, but the page does not work without both document.ready functions. I figured whoever made the double document.ready functions in the first place had some reason for doing it.
| var submission_info = {} | ||
| var submission_info = {}; | ||
| var selectedStudentCids = []; | ||
| var selectedSubmissions = []; |
There was a problem hiding this comment.
should we make these sets, would this be faster when we check for inclusion?
There was a problem hiding this comment.
some functions (e.g requestData) expect arrays, changing to sets would be slower for those
KesterTan
left a comment
KesterTan
left a comment
There was a problem hiding this comment.
Overall functionality works fine, nice that pagination finally works as expected. There's some delay in making the requests but sorting/searching still seem reasonably fast. Just have a few comments.
|
Regrade all causes the whole page to re-render, the submissions seem to be empty for a while before it is loaded again. As a result, the tooltip for regrade all also dissapears. |
The tooltip for regrade all does not disappear for me, was unable to reproduce this behavior |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/submissions_controller.rb (1)
67-68: Minor typo in comment.Line 67: "we don't stored" should be "we don't store".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/submissions_controller.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-06-10T19:38:42.782Z
Learnt from: najclark
Repo: autolab/Autolab PR: 2011
File: app/controllers/courses_controller.rb:1316-1320
Timestamp: 2024-06-10T19:38:42.782Z
Learning: The user confirmed the concern about potential security risks when setting `flash[:error]` and `flash[:html_safe]` with user-supplied input, specifically mentioning `course_name` derived from a tar file's contents. It's important to ensure proper sanitization of user-supplied input to prevent XSS vulnerabilities, especially when `flash[:html_safe]` is used, which indicates the content should not be escaped.
Applied to files:
app/controllers/submissions_controller.rb
📚 Learning: 2024-09-20T20:46:52.775Z
Learnt from: evanyeyeye
Repo: autolab/Autolab PR: 2209
File: app/controllers/autograders_controller.rb:0-0
Timestamp: 2024-09-20T20:46:52.775Z
Learning: In the `AutogradersController`, ensure that the `download_file` method has `action_auth_level :download_file, :instructor` to restrict access to instructors.
Applied to files:
app/controllers/submissions_controller.rb
📚 Learning: 2024-06-10T19:38:42.782Z
Learnt from: jlge
Repo: autolab/Autolab PR: 2011
File: app/controllers/assessments_controller.rb:28-29
Timestamp: 2024-06-10T19:38:42.782Z
Learning: `destroy_no_redirect` in `app/controllers/assessments_controller.rb` is an internal method used within the controller and not a public endpoint. It should not be included in the `before_action :set_assessment` exclusion list.
Applied to files:
app/controllers/submissions_controller.rb
🧬 Code graph analysis (1)
app/controllers/submissions_controller.rb (3)
app/models/submission.rb (1)
final_score(327-335)app/controllers/application_controller.rb (1)
action_auth_level(53-71)app/helpers/application_helper.rb (1)
computed_score(111-118)
🪛 RuboCop (1.81.7)
app/controllers/submissions_controller.rb
[convention] 136-136: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (3)
app/controllers/submissions_controller.rb (3)
42-43: LGTM: SQL injection vulnerability fixed.The order direction is now properly sanitized using a whitelist before being used in the SQL query, addressing the critical security concern from the previous review.
106-138: LGTM: Well-structured DataTables formatter.The method properly declares authorization, handles both pre-computed and dynamically-computed scores, and uses the
computed_scorehelper to safely handle any score computation exceptions. The data structure aligns with DataTables' expected format.Optional: RuboCop suggests adding a trailing comma after line 136 for consistency with Ruby style guides, but this is purely stylistic.
27-34: No changes needed—@Submissions is actively used in the view.The search results show that
@submissionsis used in multiple ways in the view:
- Line 15 iterates over
@submissionsto populate initial table data- Line 66 uses
@submissions.any?to determine button state- Line 106 passes
@submissionsto a download helperThe HTML format correctly loads submissions because they're needed for initial data population, button availability checks, and download functionality. DataTables supplements this with server-side pagination for subsequent interactions, not replacing the initial data requirement.
Note:
@submissions_to_cud(created on line 33 of the controller) does not appear to be used in the view and could potentially be removed if it's not needed elsewhere.Likely an incorrect or invalid review comment.
screen-recording-autolab-25nov.movCheck recording above, there should be a tooltip that says x number of submissions regraded but none is shown. 
|
Description
This changes implements server-side pagination for the manage submissions page to improve loading times.
Motivation and Context
Previously, it would take about 1.5 seconds for each page of the manage submissions page to load since we would render all of them at once. For example, for a submissions page with 400 submissions, it would take about 6 seconds. This change renders only one page at a time, to take a maximum of 1.5 seconds each time.
How Has This Been Tested?
Tested all of the manage submissions functions locally
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for linting